Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Change default naming to not include release name #119

Merged
merged 1 commit into from
Jul 8, 2024
Merged

Change default naming to not include release name #119

merged 1 commit into from
Jul 8, 2024

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented Jul 4, 2024 •
edited
Loading

This makes for briefer resource names, while preserving the ability to deploy multiple instances in the same namespace. It also systematically introduces helper functions rendering various resource names, which is useful if a parent chart wants to reference them. Overall, this PR is making this chart align with how the jupyterhub chart behaves.

Naming changes

# jupyterhub chart's naming
helm template jupyterhub | grep "  name:" | head -3
  name: hub
  name: proxy
  name: singleuser

helm template jupyterhub --set fullnameOverride=test | grep "  name:" | head -3
  name: test-hub
  name: test-proxy
  name: test-singleuser

helm template jupyterhub --set nameOverride=test --set fullnameOverride=null | grep "  name:" | head -3
  name: release-name-test-hub
  name: release-name-test-proxy
  name: release-name-test-singleuser


# binderhub-service chart's naming -- before PR
helm template binderhub-service | grep "  name:" | head -3
  name: release-name-binderhub-service
  name: release-name-binderhub-service-build-pods-docker-config
  name: release-name-binderhub-service

helm template binderhub-service --set fullnameOverride=test | grep "  name:" | head -3
  name: test
  name: test-build-pods-docker-config
  name: test

helm template binderhub-service --set nameOverride=test --set fullnameOverride=null | grep "  name:" | head -3
Error: values don't meet the specifications of the schema(s) in the following chart(s):
binderhub-service:
- (root): fullnameOverride is required



# binderhub-service chart's naming -- after PR
helm template binderhub-service | grep "  name:" | head -3
  name: binderhub
  name: build-pods-docker-config
  name: binderhub

helm template binderhub-service --set fullnameOverride=test | grep "  name:" | head -3
  name: test-binderhub
  name: test-build-pods-docker-config
  name: test-binderhub

helm template binderhub-service --set nameOverride=test --set fullnameOverride=null | grep "  name:" | head -3
  name: release-name-test-binderhub
  name: release-name-test-build-pods-docker-config
  name: release-name-test-binderhub

Breaking changes

Anything referencing these names will experiencing a breaking change, but if that isn't done, it may be fine to upgrade because there isn't a PV resource or similar being renamed that leads to data loss.

There is no way to exactly retain the existing naming as part of this change.

During upgrade when using an Ingress resource with ingress-nginx, you may run into an error like:

Error: UPGRADE FAILED: failed to create resource: admission webhook "validate.nginx.ingress.kubernetes.io" denied the request: host "binderhub-ui-demo.2i2c.cloud" and path "/" is already defined in ingress binderhub-ui-demo/binderhub-ui-demo-binderhub-service

To handle that, just delete the old ingress resource first manually by kubectl delete ingress ....

Related docs

The jupyterhub chart has related docs on fullnameOverride.

@consideRatio consideRatio force-pushed the pr/change-default-naming branch from f836186 to d38c2ef Compare July 4, 2024 14:14
@consideRatio consideRatio force-pushed the pr/change-default-naming branch from d38c2ef to 7580133 Compare July 4, 2024 14:28
consideRatio
consideRatio commented Jul 4, 2024
View reviewed changes
binderhub-service/templates/_helpers-labels.tpl
Comment on lines +1 to +19
{{- /*
Common labels
*/}}
{{- define "binderhub-service.labels" -}}
helm.sh/chart: {{ printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" }}
{{ include "binderhub-service.selectorLabels" . }}
{{- if .Chart.AppVersion }}
app.kubernetes.io/version: {{ .Chart.AppVersion | quote }}
{{- end }}
app.kubernetes.io/managed-by: {{ .Release.Service }}
{{- end }}

{{- /*
Selector labels
*/}}
{{- define "binderhub-service.selectorLabels" -}}
app.kubernetes.io/name: {{ .Values.nameOverride | default .Chart.Name | trunc 63 | trimSuffix "-" }}
app.kubernetes.io/instance: {{ .Release.Name }}
{{- end }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I relocated these to a dedicated file to keep track of what helper functions was related to: labels, names, and other misc things.

@consideRatio consideRatio mentioned this pull request Jul 4, 2024
Closed
@consideRatio
Copy link
Contributor Author

I did a delayed self-review, and is now moving ahead merging this based on #57 (comment) where I'll followup with testing it practically by doing 2i2c-org/infrastructure#4370

@consideRatio consideRatio merged commit 3974025 into 2i2c-org:main Jul 8, 2024
9 checks passed
consideRatio pushed a commit that referenced this pull request Jul 8, 2024
[binderhub-service] Automatic update for commit 0.1.0-0.dev.git.255.h…
5d8b406 
…7580133

#119 Merge pull request #119 from consideRatio/pr/change-default-naming
@consideRatio consideRatio mentioned this pull request Jul 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
breaking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not put name of release in resource names by default
1 participant
@consideRatio